-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enable kv interface in 2.0 #3282
Conversation
OK for me, 2 type testings:
|
@critical27 is'it possible to add an append api? |
What is the purpose? Could, but the performance will be very poor. |
Hi @critical27 plz solve the conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we support getPrefix or removePrefix ? generally LGTM
I think these pure kv interfaces are more suitable in separate client semantically? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. but why do we need to remove this GeneralStorageClient&Server? Maybe I need to modify my rpc modification to specific interface and keep a rpc server for those requests not from graph client, so that we can keep tools in different process.
We can't do it just like a put/get, because we need to fan out to do prefix/removePrefix from all partition, think them as a |
The server do need to be removed, because we bind a service to a port separately, we have already used 4 ports. We definitely don't want to add a port more, the kv interface is mainly for test only. |
Ah... The reason is we don't want to add an extra service here. (we need one more extra port to bind), so both the service and client will be removed. In 1.0, they are in the same service as well. |
Codecov Report
@@ Coverage Diff @@
## master #3282 +/- ##
==========================================
- Coverage 85.31% 85.22% -0.10%
==========================================
Files 1289 1276 -13
Lines 120073 118982 -1091
==========================================
- Hits 102442 101398 -1044
+ Misses 17631 17584 -47
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
As title. For later kv test or jepsen, @HarrisChu @kikimo, you guys could try it. Perhaps the java client need some update for jepsen.